Harden node-serviced updates - #25
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe update flow validates and stages ChangesNode-serviced transactional installation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant UpdateCommand
participant install_node_service_script
participant systemctl
participant node_service_api_ready
participant rollback_node_serviced_binary
UpdateCommand->>install_node_service_script: stage and activate validated binary
install_node_service_script->>systemctl: restart service
systemctl-->>install_node_service_script: restart result
install_node_service_script->>node_service_api_ready: poll authenticated HTTPS API
node_service_api_ready-->>install_node_service_script: readiness result
install_node_service_script->>rollback_node_serviced_binary: restore binary after failure
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pg-node.sh (1)
352-355: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a strict mode for missing checksums.
The function returns success when the release has no
checksums.txtasset. An attacker who can publish a release asset can also omitchecksums.txt, which reduces validation to the ELF magic check. Consider an opt-in variable so operators can require checksum verification.♻️ Proposed opt-in strict check
if [ -z "$checksum_url" ] || [ "$checksum_url" = "null" ]; then + if [ "${NODE_SERVICE_REQUIRE_CHECKSUM:-false}" = true ]; then + colorized_echo red "No checksums.txt asset is available and checksum verification is required." + return 1 + fi colorized_echo yellow "No checksums.txt asset is available; continuing with archive and ELF validation." return 0 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pg-node.sh` around lines 352 - 355, Add an opt-in strict checksum setting to the missing-asset branch in the checksum validation function: when enabled, fail instead of returning success if checksum_url is empty or "null"; otherwise preserve the current warning and archive/ELF validation behavior. Use the existing configuration convention for naming and reading the operator-controlled setting.tests/unit_pgnode.sh (1)
230-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the default
curlstub after the checksum tests.This stub replaces the file-level stub from line 16 and is never restored; line 355 unsets the other stubs but not
curl. Any later test that callscurlwithout-oexecutesprintf ... > ""and fails.export -f curlis also unnecessary, because the helpers run in the current shell.Run the following script to check for later
curlusage in the suite:#!/bin/bash # List curl definitions and call sites in the pg-node unit test suite. fd -t f 'unit_pgnode.sh' | xargs rg -nP -C 3 '\bcurl\b'♻️ Proposed stub restoration
-export -f curl assert_true "verify_node_serviced_checksum: accepts matching release checksum" \ verify_node_serviced_checksum "https://example.invalid/checksums.txt" "$checksum_asset" "$checksum_archive" "$checksum_dir" checksum_value="$(printf '0%.0s' {1..64})" assert_false "verify_node_serviced_checksum: rejects mismatch" \ verify_node_serviced_checksum "https://example.invalid/checksums.txt" "$checksum_asset" "$checksum_archive" "$checksum_dir" +curl() { echo ""; return 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_pgnode.sh` around lines 230 - 242, Restore the original file-level curl stub after the checksum tests, before the later tests run, so calls without -o retain their default behavior. Remove the unnecessary export -f curl from this checksum-specific stub, and ensure the existing cleanup around line 355 does not leave this replacement active.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pg-node.sh`:
- Around line 417-427: Update rollback_node_serviced_binary so the no-backup
branch explicitly reports that SERVICE_BINARY_PATH is being removed,
distinguishing it from restoration. Keep the existing rm behavior and return
handling unchanged, and ensure the message is emitted before the deletion.
---
Nitpick comments:
In `@pg-node.sh`:
- Around line 352-355: Add an opt-in strict checksum setting to the
missing-asset branch in the checksum validation function: when enabled, fail
instead of returning success if checksum_url is empty or "null"; otherwise
preserve the current warning and archive/ELF validation behavior. Use the
existing configuration convention for naming and reading the operator-controlled
setting.
In `@tests/unit_pgnode.sh`:
- Around line 230-242: Restore the original file-level curl stub after the
checksum tests, before the later tests run, so calls without -o retain their
default behavior. Remove the unnecessary export -f curl from this
checksum-specific stub, and ensure the existing cleanup around line 355 does not
leave this replacement active.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d661577-9775-4c8a-b434-53a03f843b59
📒 Files selected for processing (2)
pg-node.shtests/unit_pgnode.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit_pgnode.sh (3)
270-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin
SERVICE_NAMEfor the no-backup message assertion.
rollback_node_serviced_binarybuilds the label from${SERVICE_NAME:-node-serviced}. This assertion expects the fallbacknode-serviced. The test therefore passes only whileSERVICE_NAMEis empty at this point. Line 294 setsSERVICE_NAMElater, so a future reorder breaks this test for a reason that is hard to see.Set the variable explicitly before the call.
♻️ Proposed change
NODE_SERVICE_HAD_PREVIOUS=false NODE_SERVICE_BACKUP_PATH="" +SERVICE_NAME="" rollback_output=$(rollback_node_serviced_binary 2>&1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_pgnode.sh` around lines 270 - 277, Set SERVICE_NAME explicitly to node-serviced before invoking rollback_node_serviced_binary in the no-backup assertion, so the expected fallback label remains deterministic regardless of test ordering or later assignments.
331-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the activation before you assert the restore.
Line 333 calls
activate_node_serviced_binarywithout checking the status. If activation fails, the old binary stays at$SERVICE_BINARY_PATH. The check at line 346 then findsold-binaryand passes, although no rollback occurred. The test reports success for a broken path.Assert activation, and assert that the new binary is active before the restore.
💚 Proposed change
-activate_node_serviced_binary "$valid_binary" true +assert_true "activate_node_serviced_binary: stages the transaction candidate" \ + activate_node_serviced_binary "$valid_binary" true +assert_false "activate_node_serviced_binary: replaces the previous binary" \ + grep -q 'old-binary' "$SERVICE_BINARY_PATH"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_pgnode.sh` around lines 331 - 350, Update the test around activate_node_serviced_binary to assert that activation succeeds, then verify SERVICE_BINARY_PATH contains the newly activated binary before invoking restore_node_service_installation. Keep the existing restore assertions, ensuring the test cannot pass unless both activation and rollback occur.
289-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a quoted
API_KEY.The fixture at line 292 writes bare, unquoted values.
read_node_service_env_valuealso handles quoted values and strips trailing comments. Neither branch is covered.Add a fixture with
API_KEY= "unit#test#key"and assert that the emitted header contains the full key. That case fails today, because the comment stripping runs before the quote removal inpg-node.sh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_pgnode.sh` around lines 289 - 320, Extend the readiness fixture around wait_for_node_service_ready with a quoted API_KEY value containing # characters, such as "unit#test#key". Invoke the readiness check and assert the generated curl configuration header preserves the complete key, covering read_node_service_env_value’s quoted-value and comment-stripping behavior.pg-node.sh (1)
1791-1806: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftThe rollback does not revert the firewall change.
Line 1791 opens
$api_portbefore the installation starts. The failure paths at lines 1794 and 1803 restoreENV_FILE, the unit, and the binary. They leave the firewall rule in place. After a failed first-time installation the port stays open with no service behind it.Record whether
configure_firewall_for_portcreated a new rule, and close the port inrestore_node_service_installationwhen the installation did not exist before.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pg-node.sh` around lines 1791 - 1806, Track whether configure_firewall_for_port for api_port creates a new firewall rule before installation proceeds, and pass that state through the failure rollback paths around install_node_service_script and the service readiness checks. Update restore_node_service_installation to remove the newly created port rule when the installation did not previously exist, while preserving existing firewall rules and successful-install behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pg-node.sh`:
- Around line 262-273: Update the value parsing in the awk block to detect and
remove surrounding quotes before stripping trailing comments, so # characters
inside quoted values are preserved. Apply comment removal only to unquoted
values, while retaining the existing quote-unwrapping and output behavior for
both quote styles.
---
Nitpick comments:
In `@pg-node.sh`:
- Around line 1791-1806: Track whether configure_firewall_for_port for api_port
creates a new firewall rule before installation proceeds, and pass that state
through the failure rollback paths around install_node_service_script and the
service readiness checks. Update restore_node_service_installation to remove the
newly created port rule when the installation did not previously exist, while
preserving existing firewall rules and successful-install behavior.
In `@tests/unit_pgnode.sh`:
- Around line 270-277: Set SERVICE_NAME explicitly to node-serviced before
invoking rollback_node_serviced_binary in the no-backup assertion, so the
expected fallback label remains deterministic regardless of test ordering or
later assignments.
- Around line 331-350: Update the test around activate_node_serviced_binary to
assert that activation succeeds, then verify SERVICE_BINARY_PATH contains the
newly activated binary before invoking restore_node_service_installation. Keep
the existing restore assertions, ensuring the test cannot pass unless both
activation and rollback occur.
- Around line 289-320: Extend the readiness fixture around
wait_for_node_service_ready with a quoted API_KEY value containing # characters,
such as "unit#test#key". Invoke the readiness check and assert the generated
curl configuration header preserves the complete key, covering
read_node_service_env_value’s quoted-value and comment-stripping behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c71a8106-1f70-4cf7-a7f3-d48dd71292b9
📒 Files selected for processing (2)
pg-node.shtests/unit_pgnode.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pg-node.sh`:
- Around line 308-313: Update the value parsing logic in pg-node.sh to recognize
a quoted token followed by optional whitespace and a trailing comment before
stripping comments, preserving embedded # characters in values such as
API_KEY="unit#test-key" # deployment note. Add a regression case covering this
exact .env input and ensure the readiness probe receives the complete key.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3250cfed-198f-45f7-b410-96a6dbf83b0e
📒 Files selected for processing (2)
pg-node.shtests/unit_pgnode.sh
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
pg-node.sh (2)
830-836: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGive the supervising timeout headroom over the curl
max-time.Both
max-time(line 820) and therun_node_service_externalbound (line 836) default toNODE_SERVICE_READINESS_TIMEOUT_SECONDS. The watchdog can therefore signal curl at the same instant curl would exit on its own timeout, which makes the reported status non-deterministic between rc 28 and the signal status.Add a small margin to the outer bound.
♻️ Proposed change
- run_node_service_external "${NODE_SERVICE_READINESS_TIMEOUT_SECONDS:-5}" curl --config "$curl_config" >/dev/null + run_node_service_external \ + "$(( ${NODE_SERVICE_READINESS_TIMEOUT_SECONDS:-5} + 2 ))" curl --config "$curl_config" >/dev/null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pg-node.sh` around lines 830 - 836, Adjust the timeout passed to run_node_service_external in the readiness check to be slightly longer than the curl max-time, while keeping both based on NODE_SERVICE_READINESS_TIMEOUT_SECONDS. Preserve the existing curl configuration and readiness behavior, ensuring the supervising watchdog has headroom before terminating curl.
1053-1071: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the temporary download directory on the early failure paths.
Lines 1057, 1064, and 1070 return 1 without removing
$tmp_dir, while every later failure path callsrm -rf "$tmp_dir". Inside a transaction the guard removes it throughNODE_SERVICE_DOWNLOAD_TMP_DIR. Outside a transaction the directory leaks.Make the cleanup consistent across all failure paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pg-node.sh` around lines 1053 - 1071, Update the early failure branches in the node-serviced release resolution flow to clean up the temporary directory before returning: the curl failure, missing latest_version, and missing asset_url checks must remove $tmp_dir using the same transaction-aware cleanup mechanism as later failures, including the NODE_SERVICE_DOWNLOAD_TMP_DIR guard.tests/test_node_service_transaction_guard.sh (1)
156-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the per-case stubs so they do not leak into later cases.
Line 156 replaces
uninstall_node_service_scriptwith a stub that always returns 42. Lines 185-188 replacewrite_node_service_unitwith a stub that always fails. Neither stub is removed. Every case after line 202 runs with those definitions in place.No current case is affected. A future case that calls
install_service_commandoruninstall_service_commandwould silently take the failure path.Define these stubs inside a subshell for the case, or
unset -fthem when the case ends.Also applies to: 175-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_node_service_transaction_guard.sh` around lines 156 - 159, Scope the test-specific stubs for uninstall_node_service_script and write_node_service_unit to their individual cases so they cannot affect subsequent cases. Define each stub inside the case’s subshell or remove it with unset -f before the case ends, preserving the intended failure behavior only for the current case.tests/test_node_service_readiness_tls.sh (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the port-wait loop and the server start into helpers.
Lines 42-48, 83-89, 110-116, and 141-147 repeat the same wait loop. Lines 33-41, 75-82, 103-109, and 130-140 repeat the same certificate generation and
s_serverstart with only the subject, the SAN, and the log file changed.A
start_tls_server <cert> <key> <log>helper and await_for_port <port>helper would remove the duplication. The helper can also fail the test when the port never opens.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_node_service_readiness_tls.sh` around lines 42 - 48, Extract the repeated certificate generation and s_server startup logic into a start_tls_server helper accepting cert, key, and log arguments, and extract each repeated readiness loop into a wait_for_port helper accepting the port. Replace all four duplicated call sites with these helpers, preserving their subject/SAN and log-specific values, and make wait_for_port fail the test when the port does not open within its retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pg-node.sh`:
- Around line 2157-2160: Ensure failures from the subshell entry points
install_service_command and uninstall_service_command propagate to their
callers: either append an outer failure check at each invocation, including
uninstall_command before subsequent cleanup, and service-uninstall, or convert
the entry points back to functions so die() and exit 1 terminate the main
script. Preserve the existing command sequencing and cleanup behavior on
success.
- Around line 205-232: Update service_installed so query failures return the
expected non-installed status rather than propagating arbitrary statuses such as
2 or 3. Preserve return 0 for an installed service and return 1 only when the
service is confirmed absent, ensuring callers like restart_service_if_installed
do not invoke systemctl start, stop, status, or journalctl after a failed query.
- Around line 2299-2308: Update uninstall_service_command and
uninstall_node_service_script to validate the results of removing SERVICE_UNIT
and SERVICE_BINARY_PATH. If either removal fails, call
abort_node_service_transaction and prevent commit_node_service_transaction from
running; only commit after both removals succeed.
- Around line 745-764: The CN fallback in the certificate identity logic must
not remove and later recreate the original cert_output path. Update the fallback
around run_node_service_external to use a newly created temporary file with
create_temp_file, or retain the existing file until the lookup finishes,
ensuring cleanup occurs afterward while preserving the current failure handling.
- Around line 394-426: Update the setsid invocation in the process-supervision
block to use the wait mode, preserving the existing child_pid/child_pgid
assignments and watchdog behavior so the watched PID remains tied to the command
whose process group is terminated on timeout.
- Around line 822-823: The curl config generation in the ssl_cert and api_key
printf statements over-escapes backslashes and quotes. Update both sed
transformations to emit a single escape character, and add a regression case
covering an API key containing both a double quote and a backslash.
In `@tests/test_node_service_readiness_tls.sh`:
- Around line 103-122: Strengthen the TLS readiness test around the existing
CN-only certificate setup by inspecting the generated certificate and asserting
it has no subjectAltName before calling node_service_api_ready. Add a separate
certificate/server scenario whose SAN contains only a non-DNS/IP entry such as
an email or URI, then assert node_service_api_ready fails for that certificate,
covering the unusable-SAN branch in node_service_certificate_identity.
In `@tests/test_node_service_transaction_guard.sh`:
- Line 316: Update the descendant liveness assertions around
EXTERNAL_CHILD_PID_FILE and QUERY_DESCENDANT_PID_FILE to first verify each PID
file exists and fail explicitly when it is missing. Only run kill -0 and report
the descendant termination result after the corresponding file has been
confirmed present.
---
Nitpick comments:
In `@pg-node.sh`:
- Around line 830-836: Adjust the timeout passed to run_node_service_external in
the readiness check to be slightly longer than the curl max-time, while keeping
both based on NODE_SERVICE_READINESS_TIMEOUT_SECONDS. Preserve the existing curl
configuration and readiness behavior, ensuring the supervising watchdog has
headroom before terminating curl.
- Around line 1053-1071: Update the early failure branches in the node-serviced
release resolution flow to clean up the temporary directory before returning:
the curl failure, missing latest_version, and missing asset_url checks must
remove $tmp_dir using the same transaction-aware cleanup mechanism as later
failures, including the NODE_SERVICE_DOWNLOAD_TMP_DIR guard.
In `@tests/test_node_service_readiness_tls.sh`:
- Around line 42-48: Extract the repeated certificate generation and s_server
startup logic into a start_tls_server helper accepting cert, key, and log
arguments, and extract each repeated readiness loop into a wait_for_port helper
accepting the port. Replace all four duplicated call sites with these helpers,
preserving their subject/SAN and log-specific values, and make wait_for_port
fail the test when the port does not open within its retries.
In `@tests/test_node_service_transaction_guard.sh`:
- Around line 156-159: Scope the test-specific stubs for
uninstall_node_service_script and write_node_service_unit to their individual
cases so they cannot affect subsequent cases. Define each stub inside the case’s
subshell or remove it with unset -f before the case ends, preserving the
intended failure behavior only for the current case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b00acb42-4adf-427c-9ae5-dd23252197bf
📒 Files selected for processing (5)
.github/workflows/command-tests.ymlpg-node.shtests/test_node_service_readiness_tls.shtests/test_node_service_transaction_guard.shtests/unit_pgnode.sh
1 participant
Summary
checksums.txtbefore installingnode-serviced(an operator may explicitly setNODE_SERVICE_REQUIRE_CHECKSUM=falsefor an exceptional legacy release)GET https://127.0.0.1:$API_PORT/, the configured CA certificate andx-api-key; the key is kept out of process arguments and quoted.envvalues preserve literal#charactersservice-installtransactional for the binary, systemd unit and.env, including restoration after a failed startScope
This hardens
pg-node service-updateand script-managed service updates. It does not make a barepg-node.serviceimplement the separatenode-servicedmaintenance API required by Panel Update, and therefore cannot recover a missing or zero-byte management daemon through Panel's/node/updaterequest.Validation
bash -n pg-node.sh tests/unit_pgnode.shbash tests/test_script_update_safety.sh: 3 update/rollback scenarios passedgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes